Honour isrecursive above listall by davidjumani · Pull Request #6045 · apache/cloudstack

davidjumani · 2022-02-28T07:13:55Z

Description

Prior to this, if listall was set to true, it would recursively list all resources irrespective of the value of isrecursive. This PR fixes it

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Bug Severity

How Has This Been Tested?

Before :

(localhost) 🐱 > list accounts filter=name,domain listall=true isrecursive=false
{
  "account": [
    {
      "domain": "ROOT",
      "name": "admin"
    },
    {
      "domain": "ROOT",
      "name": "baremetal-system-account"
    },
    {
      "domain": "domain",
      "name": "domain"
    },
    {
      "domain": "sub",
      "name": "user"
    }
  ],
  "count": 4
}

After :

(localhost) 🐱 > list accounts filter=name,domain listall=true isrecursive=false
{
  "account": [
    {
      "domain": "ROOT",
      "name": "admin"
    },
    {
      "domain": "ROOT",
      "name": "baremetal-system-account"
    }
  ],
  "count": 2
}

DaanHoogland · 2022-02-28T08:19:22Z

@davidjumani , would it make sense to let isRecursive() return a different default value, depending on whether listall is given? this seems like a very elaborate way to guard against a discrepancy, introducing a lot of null.
I would think an extra bit of check in the existing getter, will greatly reduce the size of the change as well as the complexity of it, and it will limit the check to the API, where I think it belongs.

This reverts commit 12bc26a.

DaanHoogland

code looks good

nvazquez · 2022-03-05T02:40:00Z

@blueorangutan package

blueorangutan · 2022-03-05T03:19:04Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-03-05T04:05:09Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2769

nvazquez · 2022-03-05T10:19:51Z

@blueorangutan test

blueorangutan · 2022-03-05T10:21:03Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-03-05T21:53:25Z

Trillian test result (tid-3503)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40165 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6045-t3503-kvm-centos7.zip
Smoke tests completed. 80 look OK, 12 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_redundant_vpc_site2site_vpn	`Error`	243.71	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Error`	159.95	test_vpc_vpn.py
test_01_vpc_remote_access_vpn	`Failure`	77.35	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Error`	153.00	test_vpc_vpn.py
test_DeleteDomain	`Error`	151.69	test_accounts.py
test_forceDeleteDomain	`Failure`	151.60	test_accounts.py
test_01_create_lb_rule_src_nat	`Error`	0.02	test_loadbalance.py
test_02_redundant_VPC_default_routes	`Failure`	330.17	test_vpc_redundant.py
test_isolate_network_password_server	`Error`	0.05	test_password_server.py
test_router_dhcphosts	`Error`	0.04	test_router_dhcphosts.py
test_router_dhcp_opts	`Error`	0.06	test_router_dhcphosts.py
test_01_create_delete_portforwarding_fornonvpc	`Error`	1.70	test_portforwardingrules.py
test_add_multiple_admins_in_project	`Failure`	9.59	test_enable_role_based_users_in_projects.py
test_router_dns_externalipquery	`Error`	0.04	test_router_dns.py
test_router_dns_guestipquery	`Error`	0.03	test_router_dns.py
test_router_dns_guestipquery	`Error`	0.04	test_router_dnsservice.py
test_01_isolate_network_FW_PF_default_routes_egress_true	`Error`	73.52	test_routers_network_ops.py
test_02_isolate_network_FW_PF_default_routes_egress_false	`Error`	73.48	test_routers_network_ops.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true	`Error`	133.80	test_routers_network_ops.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Error`	121.72	test_routers_network_ops.py
test_delete_account	`Error`	76.80	test_network.py
test_01_port_fwd_on_src_nat	`Error`	0.02	test_network.py
test_public_ip_admin_account	`Error`	1.11	test_network.py
test_public_ip_user_account	`Error`	1.11	test_network.py
test_reboot_router	`Error`	181.73	test_network.py
test_releaseIP	`Error`	62.59	test_network.py

nvazquez · 2022-03-08T03:21:41Z

@blueorangutan test

blueorangutan · 2022-03-08T03:22:03Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-03-08T13:43:38Z

Trillian test result (tid-3523)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35918 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6045-t3523-kvm-centos7.zip
Smoke tests completed. 81 look OK, 11 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_redundant_vpc_site2site_vpn	`Error`	244.74	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Error`	150.97	test_vpc_vpn.py
test_01_vpc_remote_access_vpn	`Failure`	76.39	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Error`	162.46	test_vpc_vpn.py
test_DeleteDomain	`Error`	152.77	test_accounts.py
test_forceDeleteDomain	`Failure`	151.69	test_accounts.py
test_01_create_lb_rule_src_nat	`Error`	0.02	test_loadbalance.py
test_isolate_network_password_server	`Error`	0.04	test_password_server.py
test_router_dhcphosts	`Error`	0.04	test_router_dhcphosts.py
test_router_dhcp_opts	`Error`	0.07	test_router_dhcphosts.py
test_01_create_delete_portforwarding_fornonvpc	`Error`	1.84	test_portforwardingrules.py
test_add_multiple_admins_in_project	`Failure`	9.75	test_enable_role_based_users_in_projects.py
test_router_dns_externalipquery	`Error`	0.04	test_router_dns.py
test_router_dns_guestipquery	`Error`	0.04	test_router_dns.py
test_router_dns_guestipquery	`Error`	0.04	test_router_dnsservice.py
test_01_isolate_network_FW_PF_default_routes_egress_true	`Error`	76.39	test_routers_network_ops.py
test_02_isolate_network_FW_PF_default_routes_egress_false	`Error`	74.35	test_routers_network_ops.py
test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true	`Error`	117.42	test_routers_network_ops.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Error`	127.93	test_routers_network_ops.py
test_delete_account	`Error`	76.82	test_network.py
test_01_port_fwd_on_src_nat	`Error`	0.02	test_network.py
test_public_ip_admin_account	`Error`	1.14	test_network.py
test_public_ip_user_account	`Error`	1.12	test_network.py
test_reboot_router	`Error`	181.99	test_network.py
test_releaseIP	`Error`	62.99	test_network.py

davidjumani · 2022-03-09T05:37:02Z

@blueorangutan package

blueorangutan · 2022-03-09T05:39:02Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-03-09T06:13:41Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2803

davidjumani · 2022-03-09T08:10:30Z

@blueorangutan test

blueorangutan · 2022-03-09T08:11:02Z

@davidjumani a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr

LGTM but requires manual testing or check/add new unit/marvin tests to cover the new changes.

blueorangutan · 2022-03-09T17:26:46Z

Trillian test result (tid-3536)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31966 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6045-t3536-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez · 2022-03-09T17:36:11Z

@davidjumani can you please fix the conflicts?

davidjumani · 2022-03-10T04:37:32Z

@blueorangutan package

blueorangutan · 2022-03-10T04:38:03Z

@davidjumani a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-03-10T05:37:11Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2824

davidjumani · 2022-03-10T06:28:43Z

@blueorangutan test

blueorangutan · 2022-03-10T06:29:03Z

@davidjumani a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-03-10T15:36:34Z

Trillian test result (tid-3555)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31473 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6045-t3555-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez

LGTM - manually tested:

Test domains defined as:

Created an account per domain
As ROOT admin: isrecursive being honoured over listall:

(localcloud) 🐱 > list accounts filter=id,name,domain listall=true isrecursive=false
{
  "account": [
    {
      "domain": "ROOT",
      "id": "2ffb598d-9d5e-11ec-8765-0200346f0009",
      "name": "admin"
    },
    {
      "domain": "ROOT",
      "id": "89b3f8d1-8a9d-42fa-8249-b85dc86e29e6",
      "name": "baremetal-system-account"
    }
  ],
  "count": 2
}
(localcloud) 🐱 > list accounts filter=id,name,domain listall=true isrecursive=true
{
  "account": [
    {
      "domain": "ROOT",
      "id": "2ffb598d-9d5e-11ec-8765-0200346f0009",
      "name": "admin"
    },
    {
      "domain": "ROOT",
      "id": "89b3f8d1-8a9d-42fa-8249-b85dc86e29e6",
      "name": "baremetal-system-account"
    },
    {
      "domain": "sub",
      "id": "e7da4a05-1224-4518-9379-1096962173b2",
      "name": "user"
    },
    {
      "domain": "subsub",
      "id": "1fae52cd-bbcf-43bc-8c4b-e41c7197b349",
      "name": "subsubdomadmin"
    },
    {
      "domain": "nested",
      "id": "140e22a8-859d-4ee8-a34c-e198cad9417d",
      "name": "nesteduser"
    }
  ],
  "count": 5
}

As a user on domain sub:
if 'allow.users.view.all.domain.accounts' = true:

(localcloud) 🐱 > list accounts filter=id,name,domain listall=true isrecursive=true
{
  "account": [
    {
      "domain": "sub",
      "id": "e7da4a05-1224-4518-9379-1096962173b2",
      "name": "user"
    }
  ],
  "count": 1
}
(localcloud) 🐱 > list accounts filter=id,name,domain listall=true isrecursive=false
{
  "account": [
    {
      "domain": "sub",
      "id": "e7da4a05-1224-4518-9379-1096962173b2",
      "name": "user"
    }
  ],
  "count": 1
}

This reverts commit 7105619.

davidjumani force-pushed the fix-recursive branch 3 times, most recently from 1c8b5a0 to 74516df Compare February 28, 2022 07:37

Honour isrecursive above listall

12bc26a

davidjumani force-pushed the fix-recursive branch from 74516df to 12bc26a Compare February 28, 2022 08:15

davidjumani added 2 commits February 28, 2022 14:08

Revert "Honour isrecursive above listall"

bbd221b

This reverts commit 12bc26a.

Being smart about it

0342c0d

DaanHoogland approved these changes Feb 28, 2022

View reviewed changes

davidjumani force-pushed the fix-recursive branch from 582916d to 7b75d2f Compare February 28, 2022 10:02

Cleanup

101d2a0

davidjumani force-pushed the fix-recursive branch 2 times, most recently from 0678a01 to 101d2a0 Compare February 28, 2022 10:46

nvazquez added this to the 4.17.0.0 milestone Mar 5, 2022

nvazquez added component:api type:bug labels Mar 5, 2022

fixing npe

224b50a

davidjumani marked this pull request as ready for review March 9, 2022 08:10

yadvr approved these changes Mar 9, 2022

View reviewed changes

Merge branch 'main' of github.com:apache/cloudstack into fix-recursive

89c519e

nvazquez approved these changes Mar 11, 2022

View reviewed changes

nvazquez merged commit 7105619 into apache:main Mar 11, 2022

Pearl1594 mentioned this pull request Mar 16, 2022

travis: Fix failing list vms & snapshots tests #6124

Closed

10 tasks

nvazquez added a commit that referenced this pull request Mar 16, 2022

Revert "Honour isrecursive above listall (#6045)"

4b41958

This reverts commit 7105619.

nvazquez mentioned this pull request Mar 16, 2022

Revert "Honour isrecursive above listall" #6126

Merged

nvazquez added a commit that referenced this pull request Mar 17, 2022

Revert "Honour isrecursive above listall (#6045)" (#6126)

3fc31ca

This reverts commit 7105619.

weizhouapache mentioned this pull request Mar 21, 2022

Compute offerings are inaccessible as regular user. #6145

Closed

Uh oh!

Conversation

davidjumani commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Bug Severity

How Has This Been Tested?

Before :

After :

Uh oh!

DaanHoogland commented Feb 28, 2022

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Mar 5, 2022

Uh oh!

blueorangutan commented Mar 5, 2022

Uh oh!

blueorangutan commented Mar 5, 2022

Uh oh!

nvazquez commented Mar 5, 2022

Uh oh!

blueorangutan commented Mar 5, 2022

Uh oh!

blueorangutan commented Mar 5, 2022

Uh oh!

nvazquez commented Mar 8, 2022

Uh oh!

blueorangutan commented Mar 8, 2022

Uh oh!

blueorangutan commented Mar 8, 2022

Uh oh!

davidjumani commented Mar 9, 2022

Uh oh!

blueorangutan commented Mar 9, 2022

Uh oh!

blueorangutan commented Mar 9, 2022

Uh oh!

davidjumani commented Mar 9, 2022

Uh oh!

blueorangutan commented Mar 9, 2022

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Mar 9, 2022

Uh oh!

nvazquez commented Mar 9, 2022

Uh oh!

davidjumani commented Mar 10, 2022

Uh oh!

blueorangutan commented Mar 10, 2022

Uh oh!

blueorangutan commented Mar 10, 2022

Uh oh!

davidjumani commented Mar 10, 2022

Uh oh!

blueorangutan commented Mar 10, 2022

Uh oh!

blueorangutan commented Mar 10, 2022

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidjumani commented Feb 28, 2022 •

edited

Loading